Rethinking cgroup Memory Enforcement - #1485
Conversation
| // becomes empty and its controllers can be delegated. Within a delegated | ||
| // subtree the only member processes are our own (typically just the worker, | ||
| // when it was started inside `parent`) | ||
| function moveProcsToLeader(parent: string, logger: Logger) { |
There was a problem hiding this comment.
Rename procs to Processes
|
|
||
| // Hard memory.max ceiling (mb) applied to each child via a cgroup v2 leaf. | ||
| // Best-effort: ignored on hosts without a writable cgroup v2 hierarchy. | ||
| cgroupMemoryLimitMb?: number; |
There was a problem hiding this comment.
The cgroup limit should be derived from the memoryLimit, no? Maybe we accept a multiplier or overhead as an argument, but I think this should be automated
|
@doc-han I'd appreciate you spending an hour with this and letting me know what you think. There are pretty good docs (I hope!) in the worker and engine readmes The outstanding gotcha is permissions. This will only work if the docker image runs with |
|
I'll have to redraft this PR if we go ahead to that its clean (right now is describes the diff from the other cgroup PR, not from main). It looks like we can switch on some GKE settings to allow the cgroups to be writable, which would allow us to set |
3 participants
Re-implementation of #1467
This rework takes a different approach to cgroup enforcement than #1467.
#1467 had the engine self-provision the whole cgroup hierarchy, starting from
the real cgroup root (/sys/fs/cgroup) and building a fixed path down from
there. That requires root, and if the root cgroup is already populated (which
it always is), it relocates every process on the host/container into a leader
cgroup just to make room — a much bigger footprint than we need.
This branch instead has the engine just use whatever cgroup it's already
running in (read from /proc/self/cgroup). No root needed, no touching the
host root — we rely on the environment (container runtime, systemd Delegate=,
etc.) to have already put us somewhere writable, and only create our per-run
leaves under that.
Other simplifications that came out of this:
heap limit (--max-old-space-size) — you can toggle either independently.
ceiling is just derived from the existing run-memory setting, and there's
no legitimate case where you'd want the leaf cgroup created anywhere other
than wherever the engine already lives.
options.
(not just "is this Linux"), and is excluded from the normal test run since
it's only safe inside an isolated cgroup — running it in an ordinary dev
shell can take down more than just the test.
Getting started
Set
WORKER_ENABLE_CGROUP_ENFORCEMENTto true to enable cgroup enforcement on the worker image.Claude notes
What changed
cgroup.ts — the core rework. ensureParent and all root-hierarchy mutation deleted. New resolveSelfCgroup() reads /proc/self/cgroup to default the parent to the cgroup the worker was started in. detect() is now read-only (exists, under the mount, memory controller present, writable); prepare() delegates the memory controller with the leader move confined strictly to the given parent. Also: the rmdir-retry timer is unref'd.
pool.ts — default parent is now resolveSelfCgroup().
lifecycle.ts / events.ts — source is now forwarded on WORKFLOW_ERROR, closing the propagation gap from my review. One discovery here: EngineError already had an unused source = 'engine' field, which the PR's 'heap' | 'cgroup' silently overloads.